Add row-level geo distance check and fix documentation examples - #1510
Add row-level geo distance check and fix documentation examples#1510simplegaurav wants to merge 5 commits into
Conversation
|
All commits in PR should be signed ('git commit -S ...'). See https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits |
Adds a row-level geo check that flags values farther than a maximum geodesic distance from a reference geography. Distance is measured in meters along the WGS 84 ellipsoid via `st_distance` on GEOGRAPHY values, so the check is meaningful for global data where planar GEOMETRY distances are not. The reference accepts a literal WKT/WKB value or a Column expression, and the maximum distance accepts a number, a Column, or a SQL expression so the radius can vary per row. Null column values and null distances are skipped; unparseable column and reference values are reported separately so the message names the value that has to be fixed. Numeric distance literals are validated up front: negative, NaN, infinite and boolean values are rejected with InvalidParameterError. The convert_column / convert_reference_geometry flags default to False, matching the existing is_geo_* relationship checks, and the rendering of the offending value follows that contract - the raw value when the input is converted from WKT/WKB, st_astext when the column is already a native GEOGRAPHY. Covered by unit tests, integration tests, the all-row-geo metadata fixture, the programmatic class-based integration test, a performance benchmark, and the quality checks reference documentation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The programmatic examples for is_geo_contains, is_geo_covers,
is_geo_intersects, is_geo_touches and is_geo_within construct the rules
with DQDatasetRule, but all of these checks are registered with
@register_rule("row"). Copying the snippets as written fails:
InvalidCheckError: Function 'is_geo_within' is not a dataset-level
rule. Use DQRowRule instead.
Switch the seven affected examples to DQRowRule and add the missing
import. are_polygons_mutually_disjoint is a genuine dataset-level rule
and is left unchanged.
These examples are not exercised by
test_apply_checks_all_geo_checks_using_classes, which is why the error
went unnoticed; backfilling that coverage needs a workspace to pick
reference geometries that pass, and is left as a follow-up.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f1ef258 to
56fbfdb
Compare
ghanse
left a comment
There was a problem hiding this comment.
Very good contribution. Left a few minor comments. Need to add 1 test case.
Include GeoJSON in the documented input formats. The docstring listed the try_to_geometry formats (WKT, WKB, EWKT, EWKB) copied from the sibling relationship checks, but this check parses with try_to_geography, which also accepts GeoJSON. Updated the docstring, the reference_geometry argument description and the reference documentation table. Add integration coverage for convert_column=False. Both new tests build a native GEOGRAPHY column with try_to_geography and then leave the convert flags at their defaults, covering the pass and violation paths and exercising the st_astext rendering branch used when the column is already a GEOGRAPHY value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks for the review — all three addressed in GeoJSON in supported formats Good catch. The docstring listed the Missing Added two integration tests covering the native def test_is_geo_within_distance_native_geography_violation(skip_if_runtime_not_geo_compatible, spark):
"""A native GEOGRAPHY value outside the radius is flagged, with the value rendered via st_astext."""
point = "POINT(5.05 52.37)"
test_df = spark.createDataFrame([[point]], _GEO_SCHEMA).select(
F.call_function("try_to_geography", F.col("geom")).alias("geom")
)
condition = is_geo_within_distance("geom", F.call_function("try_to_geography", F.lit(_POINT_INSIDE)), 1000)
...They build the column with I assert on the condition column alone rather than also selecting One thing I noticed while writing those No test in the repo currently builds a native Thanks for opening #1513. |
ghanse
left a comment
There was a problem hiding this comment.
Left a few minor suggestions.
mwojtyczka
left a comment
There was a problem hiding this comment.
Automated code review for is_geo_within_distance — 5 findings inline, ranked most-severe first (WKB rendering and the string-distance validation gap are the two worth acting on; the rest are lower severity). Line numbers verified against the PR head.
mwojtyczka
left a comment
There was a problem hiding this comment.
Thank you for the PR, I left some comments
…ess review Verified against a live serverless workspace, the original design does not work: st_distance returns planar coordinate units (degrees for lon/lat data) rather than meters, and st_distance, st_distancesphere and st_distancespheroid all reject GEOGRAPHY arguments. Rebuild the check on st_distancespheroid over GEOMETRY parsed with try_to_geometry (WKT, WKB, EWKT, EWKB, GeoJSON), which returns meters on the WGS 84 ellipsoid and matches how the sibling is_geo_* checks parse their input. st_distancespheroid is defined for points only and raises on any other geometry type and on mismatched SRIDs, so guard both: - Gate the call behind `when` on st_geometrytype, and report a non-point column value or reference instead of failing the job. - Stamp both operands with SRID 4326 before measuring. try_to_geometry assigns SRID 0 to WKT/WKB and 4326 to EWKT/GeoJSON, so mixed formats would otherwise raise ST_DIFFERENT_SRID_VALUES. SRID 0 is treated as WGS 84 and 4326 used as is; any other SRID is reported rather than silently misread as degrees. Review feedback addressed in the same change: - Include GeoJSON in the documented input formats (ghanse). - Add integration coverage for convert_column=False (ghanse). - Render a too-far value via st_astext so WKB input reads as WKT, and render an unparseable value from the raw input with binary shown as hex; casting raw WKB bytes to string is not valid UTF-8 and broke result collection (ghanse, mwojtyczka). - Catch OverflowError from math.isfinite for very large distances (ghanse). - Validate numeric-string distances with the same rule as numeric literals, so "-100" and -100 behave alike (mwojtyczka). - Share operand preparation with _has_topological_relationship_precise via _prepare_geo_operands (mwojtyczka). Further hardening from an adversarial review of the redesign: - A null reference column value leaves the row unmeasurable and skipped, matching the null semantics of the other geo checks, instead of being reported as an invalid geometry. - An empty reference point is reported on every row, since it silently disabled the check. - Docstrings use italics rather than backticks for object names. Register the check in the Studio built-in rule severity seed at Low alongside its siblings, and extend the integration tests to cover every behaviour above: per-row and null radius, SQL-expression and numeric-string radius, per-row and null reference, unparseable column and reference, polygon column and reference, valid and invalid binary WKB, WKT mixed with GeoJSON in both directions, projected-SRID column and reference, empty point and empty reference, and native GEOMETRY input. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Pushed
Every behaviour above has an integration test that passes on the live workspace: the full |
|
@ghanse thanks for the two rounds of review — all five of your comments are addressed in
One thing worth your attention beyond the comments: verifying against a live serverless workspace (DBR 19.6, so not a runtime-age issue) showed the original Could you take another look when you have a moment? |
|
@mwojtyczka thanks for the review — all five findings are addressed in
The check itself was also reworked after live verification showed the Would you be able to re-review? |
Changes
Adds
is_geo_within_distance, a row-level geospatial check that reports point values farther than amaximum geodesic distance, in meters, from a reference point.
Design — every point below was verified against a live serverless workspace, not assumed
st_distancespheroid, i.e. meters on the WGS 84 ellipsoid. The planarst_distancereturns coordinate units (degrees for longitude/latitude data — 0.15 for two points 10 km apart),
and on this runtime none of
st_distance,st_distancesphereorst_distancespheroidacceptGEOGRAPHY. The check therefore operates onGEOMETRYparsed withtry_to_geometry(WKT, WKB,EWKT, EWKB, GeoJSON), exactly like the other
is_geo_*checks.st_distancespheroidraisesST_INVALID_ARGUMENTat runtime for any non-pointargument, so the call is gated behind a
whenonst_geometrytype; a polygon row is reported asis not a point geometryinstead of crashing the job.try_to_geometryassigns SRID 0 to WKT/WKB and 4326 to EWKT/GeoJSON, andst_distancespheroidraisesST_DIFFERENT_SRID_VALUESon a mismatch. Both operands are stamped4326 before measuring (SRID 0 is treated as WGS 84, 4326 is used as is, so formats can be mixed);
any other SRID is reported per row rather than silently misread as degrees.
a raw string cast of WKB is invalid UTF-8 and crashes result collection), an unparseable reference,
a non-point column value or reference, a projected SRID on either, and an empty reference (which
would otherwise silently disable the check). Null column values, null reference columns, empty
column points and null distances are skipped, matching the null semantics of the sibling checks.
distanceaccepts a number, aColumn, or a SQL expression evaluated per row. Numericliterals and numeric strings are validated up front: negative, NaN, infinite, boolean, and
float-overflowing values raise
InvalidParameterError.convert_column/convert_reference_geometrydefault toFalse, matching every sibling check.Operand preparation is shared with
_has_topological_relationship_precisethrough a new_prepare_geo_operandshelper, and the per-operand point/SRID diagnostics live in_diagnose_point_operand.Also in this PR
is_geo_*checks in the reference docs,which used
DQDatasetRulealthough all of them are registered as row rules and raisedInvalidCheckErroras written.Low, alongside its siblings.Branch: 4 commits, 9 files, +650/−15. Both review rounds from @ghanse and @mwojtyczka are addressed
in
eeb4f887; see the inline replies for the per-comment details.Linked issues
None.
Tests
Run against a live serverless workspace:
tests/integration/test_row_checks_geo.py— the full file, 75 passed, including 20 tests forthis check: inside/outside radius, per-row and null radius, SQL-expression and numeric-string
radius, per-row and null reference, unparseable column and reference, polygon column and reference,
valid and invalid binary WKB, WKT mixed with GeoJSON in both directions, projected-SRID column and
reference, empty column point and empty reference, and native
GEOMETRYinput withconvert_column=False.test_apply_checks_all_geo_checks_using_classesandtest_apply_checks_all_geo_checks_as_yaml,with the check added to
all_row_geo_checks.yaml— both pass.make app-test K=builtin_rules_seed— 23 passed.tests/unit/test_geo_check_funcs.pyplus the parameter-order contract intest_check_func_signatures.py; full unit suite green.test_benchmark_is_geo_within_distancealongside theis_geo_coversbenchmarks. Not runnablelocally on Windows (the perf fixture's 1900-01-01 start date fails in
datetime.timestamp()there,for every benchmark); CI runs it on Linux.
Local gates:
black,ruff,mypy ., andpylint src tests(10.00/10) all clean.Documentation and Demos
docs/dqx/docs/reference/quality_checks.mdx: row-level table entry, YAML example andDQRowRuleexample, plus the
DQDatasetRule→DQRowRulefix described above.